Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PackageInfos_ (#3909) #8534

Merged
merged 1 commit into from
Dec 31, 2022
Merged

Conversation

blackheaven
Copy link
Collaborator

@blackheaven blackheaven commented Oct 15, 2022

It's a first attempt, I'm open to feedback.


Please include the following checklist in your PR:

doc/cabal-package.rst Outdated Show resolved Hide resolved
@Mikolaj
Copy link
Member

Mikolaj commented Oct 15, 2022

Thanks a lot! So, does this intend to fix #3909 completely or partially? Would you be able to come up with a simple test for the functionality? (It's the easiest to copy-paste and tweak an existing PackageTest or even add a clause to one if it already tests something close enough.) A tiny changelog file would be appreciated as well.

@blackheaven
Copy link
Collaborator Author

I'm lost, which changelog should I append my contribution to?

@ulysses4ever
Copy link
Collaborator

@blackheaven there's a checklist in the PR template, it has a reference for the changelog process

@blackheaven
Copy link
Collaborator Author

Finally

@blackheaven
Copy link
Collaborator Author

Is there anything I can do to help the merge?

@chreekat
Copy link
Collaborator

chreekat commented Nov 7, 2022

Hi, the impl looks fine and I'm glad this would solve a closure bloat problem in Nixpkgs!

My feedback mostly has to do with English and words in general:

  • Info, i.e. information, is an uncountable noun like "water", so PackageInfo_ sounds better. "Informations" isn't considered a word, except (TIL) in legal settings. https://en.wiktionary.org/wiki/informations
  • There are a few typos
  • I had to go all the way to GitHub and read issue Add a Version_ module which does not depend on install dirs #3909 to understand what this patch is trying to accomplish. :D If I were you, I would create a whole new section in the manual explaining the new file. You could take the existing subsection "Accessing the package version" and promote it to a higher-level section. You could add some explanation of what you can find in that module, other than just version, and even why it was created in the first place.

@blackheaven
Copy link
Collaborator Author

Thanks for your feedback, I did my best to take them in account, however, if you can highlight my typos, I'd be grateful.

Copy link
Collaborator

@chreekat chreekat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Comment on lines 654 to 655
++ "module PackageInfo_* must include it also on the 'autogen-modules' field "
++ "besides 'exposed-modules' and 'other-modules'. This specifies that "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
++ "module PackageInfo_* must include it also on the 'autogen-modules' field "
++ "besides 'exposed-modules' and 'other-modules'. This specifies that "
++ "module PackageInfo_* must include it in 'autogen-modules' as well as"
++ " 'exposed-modules' and 'other-modules'. This specifies that "

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed it, but it's a copy-and-paste from Paths_*

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha :) The original is what I would consider "European English": used widely among non-native speakers, understandable, but different from what a native speaker would say. I almost considered just leaving it, because I think it's better to say it's dialectical rather than "wrong". Fine either way

doc/cabal-package.rst Outdated Show resolved Hide resolved
@blackheaven
Copy link
Collaborator Author

Let me know if I can do something to improve this PR, or if I can squash it

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Mikolaj
Copy link
Member

Mikolaj commented Dec 29, 2022

@blackheaven: thank you very much. Re squashing the fixup commits, if you set the squash+merge_me label, Mergify is going to squash the PR when merging it (in two days).

@blackheaven
Copy link
Collaborator Author

Thanks, I did it manually.

@Mikolaj
Copy link
Member

Mikolaj commented Dec 29, 2022

Perfect. Then go ahead and set the ordinary merge_me label.

@blackheaven blackheaven added the merge me Tell Mergify Bot to merge label Dec 29, 2022
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Dec 31, 2022
@mergify
Copy link
Contributor

mergify bot commented Dec 31, 2022

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2022, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

@andreabedini
Copy link
Collaborator

For future reference this feature should have been conditional on cabal-version. See #9331

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants